fix: support pagination and cwd filtering in list sessions#13460
fix: support pagination and cwd filtering in list sessions#13460aniruddhaadak80 wants to merge 1 commit intoNousResearch:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds pagination support to the ACP “list sessions” endpoint to address Issue 13450, enabling clients to page through session listings while still supporting cwd-based filtering via the SessionManager.
Changes:
- Adds
cursorhandling to return sessions after a given session ID. - Adds
limitsupport and computesnextCursorwhen more results are available. - Extends
ListSessionsResponseto include the computednextCursor.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| ) | ||
| return ListSessionsResponse(sessions=sessions) | ||
|
|
There was a problem hiding this comment.
There is trailing whitespace on the blank line after appending to sessions, which will show up in diffs and can trip linters. Consider removing the extra spaces.
| @@ -451,7 +467,9 @@ async def list_sessions( | |||
| updated_at=updated_at, | |||
| ) | |||
| ) | |||
| return ListSessionsResponse(sessions=sessions) | |||
|
|
|||
| next_cursor = sessions[-1].session_id if has_more and sessions else None | |||
| return ListSessionsResponse(sessions=sessions, nextCursor=next_cursor) | |||
There was a problem hiding this comment.
Pagination behavior (cursor slicing, limit, and nextCursor generation) is introduced here, but the existing ACP server tests only cover title/updated_at conversion and cwd filtering. Adding tests for: (1) limit truncation + nextCursor set, (2) passing cursor returns the next page, and (3) boundary cases like empty results would help prevent regressions.
| infos = [] | ||
|
|
||
| # Cap limit | ||
| limit = kwargs.get("limit", 50) |
There was a problem hiding this comment.
limit is pulled from kwargs without validation/capping, despite the comment “Cap limit”. If the client passes a non-int (or a negative/zero) this can raise at len(infos) > limit / infos[:limit] or produce surprising pagination behavior. Consider coercing to int, defaulting on failure, and clamping to a sensible range (e.g., 1..1000).
| limit = kwargs.get("limit", 50) | |
| raw_limit = kwargs.get("limit", 50) | |
| try: | |
| limit = int(raw_limit) | |
| except (TypeError, ValueError): | |
| limit = 50 | |
| limit = max(1, min(limit, 1000)) |
|
|
||
| next_cursor = sessions[-1].session_id if has_more and sessions else None | ||
| return ListSessionsResponse(sessions=sessions, nextCursor=next_cursor) |
There was a problem hiding this comment.
The ListSessionsResponse constructor is populated with nextCursor=... while the rest of this file consistently uses snake_case field names when instantiating ACP schema models (e.g., session_id=..., protocol_version=...). Even if aliases currently make this work, using the canonical Python field name here (likely next_cursor) would keep style consistent and reduce the chance of a runtime validation error if the schema config changes.
| next_cursor = sessions[-1].session_id if has_more and sessions else None | |
| return ListSessionsResponse(sessions=sessions, nextCursor=next_cursor) | |
| next_cursor = sessions[-1].session_id if has_more and sessions else None | |
| return ListSessionsResponse(sessions=sessions, next_cursor=next_cursor) |
|
Salvaged via #13521 (merged). Your commit was cherry-picked onto current main and landed as c1fb7b6 with your authorship preserved — thanks for the fix! Small follow-ups on top of your commit (in 4cc5065):
Closes #13450. Appreciate the contribution! |
Fixes Issue #13450. This commit adds cursor index tracking, limits, and nextCursor generation for the paginated ACP sessions list endpoint.